Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#764 - Add option to ignore delivered amounts in order group distribution #765

Merged
merged 8 commits into from
Feb 5, 2021
Merged

#764 - Add option to ignore delivered amounts in order group distribution #765

merged 8 commits into from
Feb 5, 2021

Conversation

lentschi
Copy link
Contributor

@lentschi lentschi commented Aug 23, 2020 •
edited
Loading

TODOs:

  • Translate the new config checkbox to other languages
  • Check Dutch translations.

Not TODO after all 🙂 - see #764 (comment)

@paroga paroga force-pushed the master branch 6 times, most recently from ca76347 to c6250de Compare September 5, 2020 15:00
wvengen
wvengen reviewed Oct 13, 2020
View reviewed changes
config/locales/en.yml Outdated Show resolved Hide resolved
@wvengen
Copy link
Member

wvengen commented Oct 13, 2020

I may be useful to show something on the receive screen about whether group orders are updated or not, something that doesn't catch the attention, but is there if you need to know it.

@paroga
Copy link
Member

paroga commented Oct 13, 2020

i don't think we should merge as it is and instead change it to #764 (comment)

@wvengen
Copy link
Member

wvengen commented Oct 13, 2020

Ah, sorry, you're right! 👍

@lentschi lentschi changed the title First go at #764 - Add option to ignore delivered amounts in order group distribution #764 - Add option to ignore delivered amounts in order group distribution Oct 17, 2020
@lentschi lentschi marked this pull request as ready for review October 17, 2020 18:51
@lentschi lentschi mentioned this pull request Oct 17, 2020
Open
paroga
paroga reviewed Oct 17, 2020
View reviewed changes
Copy link
Member

@paroga paroga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just two small comments, otherwise LGTM. i'll wait until we released 4.7.1 (see #777) before merging it into master

app/views/admin/configs/_tab_others.html.haml Outdated Show resolved Hide resolved
app/models/group_order_article.rb Show resolved Hide resolved
@paroga
Copy link
Member

paroga commented Feb 5, 2021

LGTM, but could you please add some model tests for it

@lentschi
Copy link
Contributor Author

lentschi commented Feb 5, 2021 •
edited
Loading

LGTM, but could you please add some model tests for it

The only model method i changed here is GroupOrderArticle#calculate_result and as far as I can see it doesn't have any real unit tests at all so far. (Except for this line implicitly calling it, but not really accessing its contained algorithms.)

I now did add two simple tests, but they merely scratch the surface of this method.
IMO it's far too big and should be split up before going into writing too many unit tests.

@paroga paroga merged commit 45a8911 into foodcoops:master Feb 5, 2021
@lentschi lentschi deleted the issue_764 branch February 5, 2021 22:46
lentschi added a commit to lentschi/foodsoft that referenced this pull request Feb 5, 2021
@lentschi
Added factory "requirements"
1668bff 
see foodcoops#765 (comment)
@lentschi lentschi restored the issue_764 branch February 5, 2021 22:56
lentschi added a commit to lentschi/foodsoft that referenced this pull request Feb 5, 2021
@lentschi
Added factory "requirements"
e2fa15a 
see foodcoops#765 (comment)
@lentschi lentschi mentioned this pull request Feb 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paroga paroga paroga left review comments

@wvengen wvengen wvengen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lentschi @wvengen @paroga